I have had to use cp to cache a dir then had to use cp.is_cached to get a path to a file. Is there a need for cp.cache_local_path (_extrn_path) so can get file name translation from source to local cache?

With regards to caching http and http, can we use If-Modified-Since or keep a copy of the header and compare key values in the header to determine of the file needs to be downloaded again. Maybe cache/.../extern_meta which contains the http/https headers for extrn_files

Other thing which might help is keep the time stamp on the file the same as the time stamp on the source if possible.

I can raise these idea's as an issue if you like.

I have had to use cp to cache a dir then had to use cp.is_cached to get a path to a file. Is there a need for cp.cache_local_path so can get file name translation from source to local cache?

cp.is_cached will give the cached path if the file is cached, and an empty string if it is not cached. Why is this not sufficient?

With regards to caching http and http seem it needs to use If-Modified-Since or keep a copy of the header and compare key values in the header to determine of the file needs to be downloaded again. Maybe cache/.../extern_meta which contains the http/https headers for extrn_files

Other thing which might help is keep the time stamp on the file the same as the time stamp on the source if possible.

I honestly have no idea what you're talking about here, sorry.

The source_hash will be used to drive whether or not the file is downloaded. If the locally-cached file matches the configured source_hash, we don't download. Before, we'd download every time, and then compare the hash to the source_hash. If it didn't match, we'd fail the state anyway, so there would be a lot of unnecessary downloading for what result in a failed state. This would allow the state to continue to work if the remote file changes, and if you want the new copy of the file, you just update the source_hash.

The follow are two example of http response headers
From GET https://the.earth.li/~sgtatham/putty/0.70/w64/putty-64bit-0.70-installer.msi

HTTP/1.1 200 OK
Date: Fri, 15 Sep 2017 03:11:50 GMT
Server: Apache
Last-Modified: Tue, 04 Jul 2017 19:36:06 GMT
ETag: "2e8600-55382fea0ec86"
Accept-Ranges: bytes
Content-Length: 3048960
Keep-Alive: timeout=5, max=99
Connection: Keep-Alive
Content-Type: application/x-msi

From inkscape.

HTTP/1.1 200 OK
Server: nginx/1.10.3
Content-Type: application/octet-stream
Last-Modified: Mon, 07 Aug 2017 16:11:49 GMT
ETag: "59889145-4f41ab7"
Expires: Tue, 10 Oct 2017 11:02:31 GMT
Cache-Control: max-age=2592000
Content-Disposition: attachment; filename="inkscape-0.92.2-x64.msi"
Content-Length: 83106487
Accept-Ranges: bytes
Date: Fri, 15 Sep 2017 03:04:31 GMT
Via: 1.1 varnish
Connection: keep-alive
X-Served-By: cache-mel6525-MEL
X-Cache: HIT
X-Cache-Hits: 0
X-Timer: S1505444671.241058,VS0,VE1

For example if Content-Length, ETag and Last-Modified have not change the existing cached file is ok.
Cache-Control could be used to clean out old files.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Modified-Since
The the putty example when requesting the file "GET"
If-Modified-Since: Tue, 04 Jul 2017 19:36:06 GMT
The web servers responds with 304 Not Modified.

Or using the etag
If-None-Match: "2e8600-55382fea0ec86"
The web servers responds with 304 Not Modified.

Or send both.

Or use HEAD first and compare and then GET (fetch) if needed, which might be easier to code.
curl -I https://the.earth.li/~sgtatham/putty/0.70/w64/putty-64bit-0.70-installer.msi

HTTP/1.1 200 OK
Date: Fri, 15 Sep 2017 03:22:01 GMT
Server: Apache
Last-Modified: Tue, 04 Jul 2017 19:36:06 GMT
ETag: "2e8600-55382fea0ec86"
Accept-Ranges: bytes
Content-Length: 3048960
Content-Type: application/x-msi

By keeping the header response from GET, it allows minion to determine of the file is valid in the cache or not.

cache/.../extern_meta/...../putty-64bit-0.70-installer.msi would contain the Header from the original GET request.

HTTP/1.1 200 OK
Date: Fri, 15 Sep 2017 03:11:50 GMT
Server: Apache
Last-Modified: Tue, 04 Jul 2017 19:36:06 GMT
ETag: "2e8600-55382fea0ec86"
Accept-Ranges: bytes
Content-Length: 3048960
Keep-Alive: timeout=5, max=99
Connection: Keep-Alive
Content-Type: application/x-msi

If the HEAD request does not match the GET Header response in cache/.../extern_meta/...../putty-64bit-0.70-installer.msi when comparing Last-Modified, ETag, Content-Length, and Content-Type then fetch the file again, instead of using the cache version. But it must contain Last-Modified or ETag or both to be able to do a valid comparison. If the header missing Last-Modified and ETag and or Content-Length general means the contents is dynamic.

My feedback is that this change is unnecessarily complex and the documentation very lacking.

1. Why rename keep to cache_source? These sorts of arbitrary changes to suit the style of a new developer make upgrading salt more painful than necessary.

2. The concept file.cached and its interaction with other file states isn't clear to me. Can't we just return the old behaviour of file states with regard to the keep attribute? That is, don't clear the file from the minion (if necessary by putting it or a hash into a new archive_hash folder -- that's poorly named by the way since it isn't specific to archives).

3. If file.cached is really important and helpful, then it needs much more documentation about how to use it in conjunction with other file states.

@terminalmage I'm wondering here about 2017.7 versus develop. Seems like the latter might be a better target but let me know your thoughts.

Why rename keep to cache_source? These sorts of arbitrary changes to suit the style of a new developer make upgrading salt more painful than necessary.

It's not an arbitrary change. The new caching behavior doesn't apply just to the archive.extracted state, it also applies to the file.managed state. An argument named keep doesn't make any sense in file.managed. It's just not descriptive enough, and I think I can safely make that declaration having been a contributor for almost six years, and having been employed full-time by SaltStack for four of those years.

I'm more than willing to have my fellow long-time contributors weigh in on this though, and use keep in both archive.extracted and file.managed. Or, perhaps, I can modify the code such that either keep or cache_source are valid arguments to archive.extracted, and both do the same thing. @cachedout thoughts?

The concept file.cached and its interaction with other file states isn't clear to me. Can't we just return the old behaviour of file states with regard to the keep attribute? That is, don't clear the file from the minion (if necessary by putting it or a hash into a new archive_hash folder -- that's poorly named by the way since it isn't specific to archives).

Using a file.managed state to download the archive was never a good idea in the first place. The reason for this is because it first downloads the file into the cachedir, and then places a copy of it in the final destination. Managing files within the cachedir using file.managed is just not a good idea, that's not what it was designed to do. So, to work around this, the archive.extracted state would create a separate path to use as the final destination for the cached archive file.

The new file.cached state simplifies what we need to do to ensure that the archive file is present in the cachedir by simply ensuring that copy has been cached using cp.cache_file, without the need to devise an alternate destination for the cached archive.

With regard to the archive_hash directory being "poorly named", I'm sorry but you are just incorrect. This is a brand new directory being added just for the archive.extracted state, and it is only used if the source_hash_update argument is set to True. source_hash_update is used to force the archive to be extracted if the file paths within the archive don't change but the hash of the source archive does. In the past, we would place this hash file alongside the separate path we had created for the copy of the archive we had downloaded using file.managed. Since we're not using file.managed anymore for this, we need a new home for these hash files, and placing them in the normal path under the cachedir could result in collisions if we do a cp.cache_file on a URL which would end up being cached to the same path. Thus, the new archive_hash directory which, yes, is specific to archives.

If file.cached is really important and helpful, then it needs much more documentation about how to use it in conjunction with other file states.

Since it's designed to be used internally within state functions, and not "in conjunction" with them, I don't know how critical adding a bunch of documentation is. I can by all means add a couple code examples, but this state will rarely be useful in an SLS file, and I have already mentioned in the docstring for file.cached that it is designed for internal use within states.

I've added some code examples to the docstring for the file.cached state. I also noticed an issue with invoking a state function via __states__ instead of via a HighState instance (as we used to do), which I've also corrected.

@cachedout The unnecessary re-downloading started happening because of changes to file.managed in 2017.7.0, so I considered this a regression that needed fixing in the 2017.7 release branch. If you'd rather push this to the next feature release though, I can understand.

@terminalmage I suppose it depends on our view of performance regressions. My view here is that changes to the file client are so fundamental to the behavior and stability of Salt that I like to take the more conservative route and defer them to major releases. I'm not necessarily opposed to merging it here but I'd feel more comfortable in the develop branch.

@cachedout I don't have an objection to rebasing against develop.

@terminalmage

An argument named keep doesn't make any sense in file.managed. It's just not descriptive enough, and I think I can safely make that declaration having been a contributor for almost six years, and having been employed full-time by SaltStack for four of those years.

Thank you for all your work on this project. It has been something very important to me for the last two years.

I come from the perspective of a user of salt, who struggled to first learn and understand its concepts and live in fear of every upgrade and what part of my configuration will be broken. I love the concepts in salt, but hate its fluid nature and lack of documentation, particularly examples of best practice.

With that out of the way, I get that you feel 'cache_source' is clearer, but to me it isn't. That's just one person so make of it what you will. When I read that I see "cache" as a noun, not a verb, so I expect to put a path as the value, not a boolean. "keep" is always a verb. But more importantly, breaking my working states with each upgrade because of a better name isn't very helpful to your users. It might be that "ls" isn't the most intuitive way to get a directory listing in Unix, there is no discussion about changing it now.

Remember that every time you change some naming you break not only everyone's working system (yes, everyone should read the release notes in detail and understand every implication, but they don't), but also you invalidate all documentation, forum posts and other information already out there.

Perhaps this is a little painting the bike shed, but for something as business critical as a configuration management system, clarity and consistency are so so important.

Thanks for listening, and thanks for your detailed explanation. I hope that you could put even a fraction of what you explained to me in the official documentation. Your description of archive_hash could go into the docs almost as is. Everyone would be much the wiser.

The keep usage still works, albeit with a warning. It doesn't "break everyone's working system".

I hope that you could put even a fraction of what you explained to me in the official documentation. Your description of archive_hash could go into the docs almost as is. Everyone would be much the wiser.

Buy why? The choice of where we store the hash files tracked by source_hash_update (i.e. archive_hash, as changed in this PR) is not user-facing. It's entirely internal. What is the benefit of explaining something like this when it will almost certainly never need to be interacted with by users? The docs are a user reference, not a developer's reference.

I've modified the PR so that cache_source is now keep_source, and I've gotten rid of the warning when keep is used. If both keep_source and keep are used, keep is ignored.

@cachedout I've also rebased onto develop. I think I have a way of incorporating the non-fileclient fixes from this PR against 2017.7, which I can do in a separate PR.

Etag support has been implemented: #61764 #61391